-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test TT is not enforced when taking an element out of a TT realm to a… #46432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work as required. You need to have the element inside the iframe (and have TT enforced in the iframe not the parent document) and then move it to the parent. CSP inherits down into the iframe as currently written so this will fail
37409ee
to
27e7b24
Compare
… non-TT realm. See discussions at w3c/trusted-types#425 (comment).
Updated. PTAL. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not finished reviewing, but changes are required.
sourceFrame.addEventListener( | ||
"load", | ||
t.step_func_done(() => { | ||
testCases.forEach(c => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running all test cases in one step_func_done
results in a test suite which is hard to debug. Consider changing this file to run one async_test
or promise_test
for every element of testCases
.
"load", | ||
t.step_func_done(() => { | ||
testCases.forEach(c => { | ||
const elementId = c[0].concat('.').concat(c[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-picking: c.join(".")
would be more concise.
<meta charset="utf-8"> | ||
<meta | ||
http-equiv="Content-Security-Policy" | ||
content="require-trusted-types-for 'script';" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semicolon at the end is superfluous.
]; | ||
|
||
const sourceFrame = document.createElement("iframe"); | ||
sourceFrame.srcdoc = iframePolicy.createHTML( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sourceFrame
, at the time of assigning has no CSP, hence wrapping iframe_srcdoc
in a TrustedHTML
object is superfluous. Please remove that.
testCases.forEach(c => { | ||
const elementId = c[0].concat('.').concat(c[1]); | ||
const sourceElement = sourceFrame.contentWindow.document.getElementById(elementId); | ||
const testAttr = sourceElement.attributes[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of .attribute
's values my differ among browsers, see https://developer.mozilla.org/en-US/docs/Web/API/Element/attributes.
testAttr.name | ||
); | ||
sourceElement.removeAttributeNode(sourceAttr); | ||
// Now `sourceElement`'s node document's global belongs to a non TT-realm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is false at that line. It should be true after appending to document.body
, be moved there and rephrased to something like:
"Now sourceElement
's node document's global should belong to a non TT-realm. Hence setAttributeNode
and setAttributeNS
with non-trusted input should pass".
<head> | ||
<meta charset="utf-8" /> | ||
<script src="/resources/testharness.js"></script> | ||
<script src="/resources/testharnessreport.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the relevant spec PR (whatwg/dom#1268) is merged, the relevant spec should be linked here via <link rel="help" href="<linkToSpec>">
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ziransun: took over the patch to https://phabricator.services.mozilla.com/D231921. Please close this review.
…s-globals-CSP-after-adoption-from-TT-realm.html>. Replaces <#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1907849 gecko-commit: 5a48fdcefee755d543af0a415b6e1e216c853845 gecko-reviewers: smaug
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug Replaces <web-platform-tests/wpt#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921
…s-globals-CSP-after-adoption-from-TT-realm.html>. Replaces <#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1907849 gecko-commit: 5a48fdcefee755d543af0a415b6e1e216c853845 gecko-reviewers: smaug
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug Replaces <web-platform-tests/wpt#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug Replaces <web-platform-tests/wpt#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921 UltraBlame original commit: 5a48fdcefee755d543af0a415b6e1e216c853845
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug Replaces <web-platform-tests/wpt#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921 UltraBlame original commit: 5a48fdcefee755d543af0a415b6e1e216c853845
…node-documents-globals-CSP-after-adoption-from-TT-realm.html>. r=smaug Replaces <web-platform-tests/wpt#46432>. Differential Revision: https://phabricator.services.mozilla.com/D231921 UltraBlame original commit: 5a48fdcefee755d543af0a415b6e1e216c853845
… non-TT realm.
See discussions at w3c/trusted-types#425 (comment).